Tweak lang-mustache factory (#92211)#4
Conversation
This commits makes a few very minor tweaks to our Mustache scripting capabilities.
1. Switches from `DefaultMustacheFactory` to `SafeMustacheFactory`
In the event that the security manager were disabled (or removed, as is threatened in subsequent JDK releases), Mustache's "partial template" (`{{>partial}}`) support is a security risk because it would allow reading from an arbitrary URL or file on disk. Switching to the "Safe" version and passing in an empty set into the parent constructor disallows using partial Mustache templates. This also switches from `Function<String, String>` to the `TemplateFunction` value for the built-in functions.
3. Minor internal optimization
This removes useless grouping for one of the built-in mustache functions, and removes the call to `CollectionUtils.ensureNoSelfReferences` used in `CustomReflectionObjectHandler`. This check during stringification should not be necessary because Mustache templates without "partial" support cannot be self-referencing.
📝 WalkthroughWalkthroughThe changes refactor the Mustache templating infrastructure in CustomMustacheFactory to inherit from SafeMustacheFactory instead of DefaultMustacheFactory, replace Function<String, String> return types with TemplateFunction, and update URL encoding. Concurrently, self-reference validation is moved from CustomReflectionObjectHandler to Payload.java, with corresponding test expectations updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java`:
- Line 149: The abstract method createFunction currently returns
mustache.TemplateFunction which causes lambda return values to be re-parsed as
Mustache templates; update the API so these lambdas are plain-text functions:
change the return type of createFunction to
java.util.function.Function<String,String> (or otherwise return a plain text
Function) and adjust implementations for ToJsonCode and JoinerCode to implement
Function<String,String> so their outputs are not re-parsed as templates; update
any call sites that expect TemplateFunction to accept Function<String,String>
and ensure ToJsonCode and JoinerCode produce safe literal strings (no mustache
re-parsing).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.javamodules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.javax-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.javax-pack/plugin/watcher/qa/rest/build.gradlex-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml
💤 Files with no reviewable changes (1)
- modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java
🧰 Additional context used
🧬 Code graph analysis (1)
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java (1)
server/src/main/java/org/elasticsearch/common/util/CollectionUtils.java (1)
CollectionUtils(31-294)
🔇 Additional comments (6)
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java (1)
40-42: Good guard against self-referential payloads.
Clear, targeted validation and a helpful message hint.x-pack/plugin/watcher/qa/rest/build.gradle (1)
39-44: Skip entry aligns with updated exception source.x-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml (1)
40-43: Updated skip and assertions correctly reflect the new failure path.Also applies to: 82-83
modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java (3)
264-264: Sanity-check custom delimiter parsing after regex tweakThe simplification should be behavior-neutral; please confirm with a small test that
join delimiter='...'still parses and rejects invalid variants as expected.
361-361: Java baseline confirmed as Java 17, which fully supportsURLEncoder.encode(String, Charset). No action needed.
17-19: SafeMustacheFactory constructor correctly disables partial resolutionThe implementation is sound. In mustache.java 0.9.10, the
SafeMustacheFactory(Set<String> allowedResourceNames, String resourceRoot)constructor exists and works as intended:Collections.emptySet()as the whitelist means no template/partial names are allowed to load, effectively blocking all partial resolution from the filesystem or classpath regardless of the resource root parameter.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } | ||
|
|
||
| protected abstract Function<String, String> createFunction(Object resolved); | ||
| protected abstract TemplateFunction createFunction(Object resolved); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CustomMustacheFactory.java" -type fRepository: Signal65/elasticsearch-CodeRabbit
Length of output: 175
🏁 Script executed:
cat -n modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java | head -100Repository: Signal65/elasticsearch-CodeRabbit
Length of output: 4362
🏁 Script executed:
cat -n modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java | sed -n '140,160p'Repository: Signal65/elasticsearch-CodeRabbit
Length of output: 1042
🏁 Script executed:
cat -n modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java | sed -n '180,220p'Repository: Signal65/elasticsearch-CodeRabbit
Length of output: 2028
🏁 Script executed:
cat -n modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java | sed -n '235,260p'Repository: Signal65/elasticsearch-CodeRabbit
Length of output: 1100
🌐 Web query:
mustache.java 0.9.10 TemplateFunction interface documentation behavior
💡 Result:
In mustache.java 0.9.10, TemplateFunction is a marker interface for section lambdas that tells the engine to treat the lambda’s return value as Mustache source and compile/render it again (i.e., “reparse the results again”). [2]
This contrasts with using a plain Java 8 Function<String,String> for a section ({{#func}}...{{/func}}):
Function<String,String>: the section body is rendered first (post-substitution) and then passed toapply; the returned string is inserted as text (not recompiled as a Mustache template). [1]TemplateFunction: the lambda is invoked in the pre-substitution mode; the returned string is treated as Mustache template text and is reparsed/re-rendered against the current context. [1], [2]
Practical implication: implement TemplateFunction when you want your lambda to generate Mustache tags (e.g., return "Hello {{name}}") and have those tags evaluated; use plain Function when you only want to transform already-rendered text. [1], [2]
🌐 Web query:
mustache.java TemplateFunction vs Function lambda section rendering escaping
💡 Result:
In mustache.java there are two “lambda section” styles, and they differ mainly in when rendering happens (which directly affects escaping):
Function<String,String> (Java 8 Function) = post-substitution
- mustache.java renders the section body first (i.e.,
{{name}}etc. inside the section are already expanded/escaped), then passes the resulting text to yourFunction(“post-substitution”). (github.com) - The
Function’s returned string is then written out as plain text (it is not re-parsed as a Mustache template). (github.com)
Escaping implication: anything inside the section like{{name}}was already escaped by the normal rules before your function sees it; your function output isn’t “auto-escaped” afterward (it’s just output).
TemplateFunction = pre-substitution + reparse
TemplateFunctionexists specifically so mustache.java will reparse (re-render) the function’s returned string as a Mustache template. (javadoc.io)- mustache.java describes this as “reparse the results … again” and contrasts it with
Functionpost-substitution behavior. (github.com)
Escaping implication: because the returned string is treated as a template, any{{var}}that appears in what you return will be rendered with normal Mustache escaping rules during that re-render (and{{{var}}}/{{& var}}would be unescaped per Mustache rules). (github.com)
If you tell me your exact template snippet + which type you’re using, I can spell out precisely what gets escaped at each step.
Citations:
- 1: https://github.com/spullara/mustache.java
- 2: https://github.com/spullara/mustache.java
- 3: https://javadoc.io/static/com.github.spullara.mustache.java/compiler/0.9.6/com/github/mustachejava/TemplateFunction.html?utm_source=openai
- 4: https://github.com/spullara/mustache.java
- 5: https://github.com/janl/mustache.js/?utm_source=openai
Investigate whether TemplateFunction is the correct type for these lambdas
In mustache.java 0.9.10, TemplateFunction causes the lambda's returned string to be re-parsed as Mustache template source, whereas Function<String,String> treats the return value as plain text. This distinction is critical:
ToJsonCode(lines 190-214): Returns JSON-formatted strings that may contain{{characters. If re-parsed as templates, literal JSON syntax could be misinterpreted as Mustache directives.JoinerCode(lines 241-255): Returns concatenated strings. If delimiters or items contain{{, they would trigger template re-parsing unintentionally.
Verify that TemplateFunction is the intended type (not a regression from a prior Function<String,String> implementation) and confirm these lambdas do not produce output that would cause unintended re-parsing.
🤖 Prompt for AI Agents
In
`@modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java`
at line 149, The abstract method createFunction currently returns
mustache.TemplateFunction which causes lambda return values to be re-parsed as
Mustache templates; update the API so these lambdas are plain-text functions:
change the return type of createFunction to
java.util.function.Function<String,String> (or otherwise return a plain text
Function) and adjust implementations for ToJsonCode and JoinerCode to implement
Function<String,String> so their outputs are not re-parsed as templates; update
any call sites that expect TemplateFunction to accept Function<String,String>
and ensure ToJsonCode and JoinerCode produce safe literal strings (no mustache
re-parsing).
2 participants
PR_014
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.